tests: Add adapter-level tests for merels via game_handler (fixes #433) #891
Conversation
|
The tests seem fine. I think the issue called for the existing |
|
Thanks for the reminder, I started in a separate file for convenience while validating the adapter-flow checks and to keep the diff small for review. I’ve now merged the tests into |
Mirror the style used by connect_four to exercise the GameAdapter flow for Merels. Use stable fragments to avoid brittleness. Included: - help shows Merels help - start game posts an invite with “wants to play”, “Merels”, “join” - join triggers start message (containment) - light checks for MerelsMessageHandler helpers No production changes; tests only. Fixes zulip#433.
| ) | ||
|
|
||
| # FIXME: Add tests for computer moves | ||
| # FIXME: Add test lib for game_handler |
There was a problem hiding this comment.
Do your new tests do either of these things?
There was a problem hiding this comment.
I just pushed a commit that addresses the test lib for game_handler. However, I did not add tests for computer moves because in merels.py, it shows that supports_computer = False, so I left a comment in test_merels.py to clarify that. I assumed I wasn't supposed to modify merels.py, which is why I went this route.
Combine the adapter-flow tests with the existing Merels bot tests to keep the suite cohesive, mirroring connect_four. Replace brittle exact-string checks with stable substring assertions. Remove obsolete FIXME/TODO notes. No production code changes; tests only. All suites pass locally. Fixes zulip#433.
sulahmed20
force-pushed
the
merels/rework-tests-adapter
branch
2 times, most recently
from
26dd45f to
daecdc3
Compare
| self.assertIn("play game", help_text) | ||
| self.assertIn("quit", help_text) | ||
| self.assertIn("rules", help_text) | ||
| # Present today; OK if dropped in future wording changes. |
There was a problem hiding this comment.
Don't remove comments like this; did you generate this code with AI? It often removes comments randomly.
There was a problem hiding this comment.
Thanks for catching that, I deleted that line by accident manually. Just restored it.
| calls["n"] += 1 | ||
| return original(*args, **kwargs) | ||
|
|
||
| bot.model.make_move = types.MethodType(_wrapped_make_move, bot.model) # type: ignore[attr-defined] |
There was a problem hiding this comment.
Can you explain why you did this weird mocking strategy here? It seems very hacky and I'm not sure what you're actually testing as a result. I think the goal here would usually be to just verify that the output of the actual bot's make_move function.
There was a problem hiding this comment.
The goal of that approach was to confirm the adapter routed a move command into the model. It was a bit indirect, thanks for pointing that out. I just replaced it with an output-based, end-to-end test that verifies the adapter path directly:
- Start a 2-player game (
startgame, thenjoin) - Send move 1,1 (try both players to avoid assuming turn order)
- Assert the reply contains the handler’s stable move-ack fragment (
:move1,1, viagame_message_handler.alert_move_message)
It confirms that the adapter (1) parses the user command, (2) drives the real handler/model, and (3) emits the correct user-visible acknowledgement. It intentionally does not test game-rule correctness or internal board state—just the adapter’s “move” wiring and reply contract.
Merels previously had adapter-focused tests for help/start/join, but the move path through GameAdapter was untested. This adds a small in-file test helper to drive the adapter and a test that starts a 2-player game, sends a move, counts model.make_move calls, and asserts that the bot replies. This covers the 'test lib for game_handler' FIXME. I did not add 'computer move' tests, because the Merels bot declares supports_computer = False; there is no single-player/computer flow to exercise. I left a comment in above TestMerelsAdapter that clarifies this. No production changes; tests only. Passes local pytest, mypy, and lint. Fixes zulip#433.
sulahmed20
force-pushed
the
merels/rework-tests-adapter
branch
from
daecdc3 to
08aa6e2
Compare
|
Squashed and merged, thanks @sulahmed20! |
|
For next issues, I'd suggest working on issues that are improving the framework, rather than individual bots, which as mentioned before are largely technology demos. @Niloth-p may be able to help you find some good choices, though there's plenty of older issues open that are fairly low-hanging. |
3 participants
tests: Add adapter-level tests for Merels via game_handler.
Mirror the style used by
connect_fourto exercise theGameAdapterflow for Merels. Assert on stable fragments to avoid brittle failures
when wording changes.
Included:
MerelsMessageHandlerhelpers:parse_boardidentityget_player_coloralert_move_messageformattingOut of scope:
Rationale / approach:
connect_four/test_connect_four.pystructure (BotTestCase,transcript scanning) to test the adapter path users exercise in practice.
How did you test this PR?
Notes:
configuration; it only adds tests under
zulip_bots/.../merels/.Fixes #433.